🔵 [Low] ECC OOM injection loop can pass vacuously · Weak or missing assertions

The loop is bounded by the counted totalAllocCount with no assertion that it is non-zero. In builds where the decode-plus-derive path allocates nothing in the armed window (e.g. ECC_CACHE_CURVE plus WOLFSSL_SP_NO_MALLOC, no WOLFSSL_SMALL_STACK), the body never executes and the test reports success without exercising any allocation-failure path.

Fix: Add ExpectIntGE(totalAllocCount, 1) before the loop, as mldsa_oom_derive_fail_level() does.

🟠 [High] Three undisclosed sword32-overflow fixes in ML-DSA small-mem arithmetic ship with no executing test
🚫 BLOCK test

Beyond the stated scope of the PR, the diff adds three new mldsa_poly_red() calls under WOLFSSL_MLDSA_SMALL, immediately before mldsa_invntt_full(): in mldsa_calc_t_small_mem (wc_mldsa.c:8773), in mldsa_sign_with_seed_mu (wc_mldsa.c:9926), and in the small-mem mldsa_verify_with_mu (wc_mldsa.c:11020). Each mirrors an existing mldsa_vec_red() in the corresponding standard path (wc_mldsa.c:8630, 9763, 10771) that the small-mem paths were missing. These are real latent correctness fixes: without the reduce, accumulated products can overflow sword32 before the inverse NTT, producing wrong keys and wrong signatures in WOLFSSL_MLDSA_SMALL + small-mem builds. The supporting guard widening at wc_mldsa.c:6298 and wc_mldsa.c:8289 (dropping !defined(WOLFSSL_MLDSA_VERIFY_SMALL_MEM)) exists solely to make mldsa_red/mldsa_poly_red compile for the new verify call site, confirming the additions are deliberate. Neither the PR description nor any of the four commit messages mentions them, no test fails without them, and no CI job would notice: --enable-mldsa=yes,small appears in exactly one entry (noasm-mldsa-align0, .github/workflows/wolfCrypt-Wconversion.yml:62) which sets no *_SMALL_MEM macro, and every ML-DSA small-mem entry (smallstack-noasm-small-mem, smallstack-intelasm-precalc, precalc-a-no-int128, verify-only-m32) is "check": false with --disable-crypttests, which .github/scripts/parallel-make-check.py:285 documents as skipping the make-check phase entirely. So WOLFSSL_MLDSA_SMALL combined with any small-mem variant is never built, and the small-mem paths that are built are never executed - this is exactly the code the PR rewrites wholesale into mldsa_calc_t_small_mem and then adds a second consumer for in wc_MlDsaKey_MakePublicKey.

Suggestion: Split the three mldsa_poly_red() additions into their own commit describing the overflow they fix, and add a CI entry that both builds and runs the combination:

{"name": "mldsa-small-small-mem", "minutes": 1,
"configure": ["--enable-cryptonly", "--enable-all-crypto", "--enable-mldsa=yes,small",
"CPPFLAGS=-DWOLFSSL_MLDSA_SIGN_SMALL_MEM -DWOLFSSL_MLDSA_VERIFY_SMALL_MEM -DWOLFSSL_MLDSA_MAKE_KEY_SMALL_MEM"],
"check": true},

Recommendation: Separate the arithmetic fixes from the refactor so they are individually reviewable, and add at least one CI configuration combining WOLFSSL_MLDSA_SMALL with the small-mem macros that actually runs make check (the KATs in test_mldsa.c would catch a wrong t/w). Without a running config, both these fixes and the entire mldsa_calc_t_small_mem rewrite land unexercised.

Did not reproduce any overflows, labeling this as a defensive change that follows convention. Added suggested CI config

🟡 [Medium] Under WOLF_CRYPTO_CB_FIND the devId gate in EccDerivePubBestEffort is still ineffective, so every SEC1 private-key decode fires an unsolicited EccMakePub callback
💡 SUGGEST bug

Sites: wolfcrypt/src/asn.c:33196; wolfcrypt/src/asn.c:33197-33202
The helper skips derivation only when key->devId != INVALID_DEVID, which is exactly backwards for WOLF_CRYPTO_CB_FIND builds: there a find callback owns keys that carry INVALID_DEVID, so those keys fall straight through to wc_ecc_make_pub_ex, and ecc_make_pub_ex deliberately drops the same devId gate under FIND (ecc.c:5722-5726) to issue wc_CryptoCb_EccMakePub(key, pub). The in-tree comment justifying that FIND behaviour (ecc.c:5706-5720) reasons explicitly about the signing path - "Reaching the software signer means the device already declined to sign (the whole-sign offload wc_CryptoCb_EccSign ran first)" - and concludes a find callback would likewise decline. That precondition does not exist during SEC1 key parsing, which is a brand-new call context this PR introduces for that callback. A find callback that answers EccMakePub from a resident device key rather than the supplied key->k therefore yields the wrong public point on decode, silently and with a 0 return. Separately, every other device-first branch in ecc.c wraps the devId test in #ifndef WOLF_CRYPTO_CB_FIND (ecc.c:4775, 5721, 5916, 7035, 9150, 10622, 11232, 11822), so the helper also breaks the file's own convention. The identical omission is repeated in the new ML-DSA entry point at wolfcrypt/src/wc_mldsa.c:11453, whose comment explicitly claims key->k may be a device handle - precisely the FIND case. This is the third review round in which the WOLF_CRYPTO_CB_FIND half of this issue has been raised; only the WOLF_CRYPTO_CB_ONLY_ECC exclusion landed.

Also, at wolfcrypt/src/asn.c:33197-33202 (Low, bugs scan) — devId early-return in EccDerivePubBestEffort does not cover WOLF_CRYPTO_CB_FIND:

The helper skips derivation for device-tagged keys by testing key->devId != INVALID_DEVID. But ecc_make_pub_ex() drops exactly that gate when WOLF_CRYPTO_CB_FIND is defined (ecc.c:5721-5725): #ifndef WOLF_CRYPTO_CB_FIND / if ((err == MP_OKAY) && (key->devId != INVALID_DEVID)) / #else / if (err == MP_OKAY) / #endif. So in a WOLF_CRYPTO_CB_FIND build, the keys the helper deliberately lets through (devId == INVALID_DEVID) are precisely the ones that then reach wc_CryptoCb_EccMakePub() anyway. This PR therefore introduces a crypto-callback invocation on every private-only SEC1 key decode in such builds — a call that did not previously happen at decode time at all. The hazard is already documented in the existing comment at ecc.c:5705-5720 ("if the device ignores key->k ... it returns dG instead of the requested kG"); this change widens that exposure from the ECDSA sign path to ordinary key loading. This point was raised in an earlier review round and the exclusion was applied to the test guard in test_asn.c but not to the library guard.

Suggestion:

Recommendation: Apply the #ifndef WOLF_CRYPTO_CB_FIND wrapper in both EccDerivePubBestEffort and wc_MlDsaKey_MakePublicKey. The early return in the helper is the only place this can be fixed - gating inside ecc_make_pub_ex would change the sign path too.

[wolfcrypt/src/asn.c:33197-33202] Skip the derivation outright under WOLF_CRYPTO_CB_FIND, since the devId gate cannot be relied on there:

#if defined(PLUTON_CRYPTO_ECC) || defined(WOLF_CRYPTO_CB)
/* Don't derive host-side public points for devId-tagged keys. Under
* WOLF_CRYPTO_CB_FIND, ecc_make_pub_ex() routes even INVALID_DEVID keys
* to a find callback, so skip the derivation entirely. */
#ifdef WOLF_CRYPTO_CB_FIND
return;
#else
if (key->devId != INVALID_DEVID) {
return;
}
#endif
#endif

Alternatively add !defined(WOLF_CRYPTO_CB_FIND) to the WOLFSSL_ECC_DERIVE_PUB_BEST_EFFORT guard at asn.c:33172-33179, mirroring the !defined(WOLF_CRYPTO_CB_ONLY_ECC) exclusion already present there.

🟡 [Medium] Two sign-only ECC decode sites (pkcs7.c, ocsp.c) were not converted to derivePub=0
💡 SUGGEST convention

The PR systematically converted probe-only and sign-only decode sites to EccPrivateKeyDecodeEx(..., 0) (src/internal.c:33097, src/sniffer.c:2843, src/ssl_api_hs.c:1269, src/ssl_api_pk.c:2379, src/ssl_load.c:575, wolfcrypt/src/asn.c:9620, asn.c:10270, evp_pk.c:201, pkcs7.c:8676) but missed two meeting the same criteria. wc_PKCS7_ImportECC (wolfcrypt/src/pkcs7.c:2150) decodes a key used only for signing, and the cost is doubled: the immediately-following wc_ecc_check_key() now sees ECC_PRIVATEKEY instead of ECC_PRIVATEKEY_ONLY and runs the full public-key validation (_ecc_validate_public_key, ecc.c:11395), adding a second scalar multiply for the order check on every wc_PKCS7_EncodeSignedData. src/ocsp.c:2455 decodes ca->key.ecc, consumed only by OcspResponseEncode at src/ocsp.c:2766 for signing. Note the PKCS#7 site does gain a genuine fix from the derivation (its wc_ecc_check_key previously failed with ECC_INF_E on a private-only key), so if it is left deriving that should be stated as the reason rather than left as an oversight.

Suggestion:

Recommendation: Convert src/ocsp.c:2455 to EccPrivateKeyDecodeEx(..., 0), and either convert wolfcrypt/src/pkcs7.c:2150 or add a comment stating the derivation is kept deliberately so wc_ecc_check_key succeeds on private-only keys.

🔵 [Low] WC_MLDSA_HAVE_CHECK_KEY's second arm is unreachable and the condition is duplicated in two spellings
💡 SUGGEST convention

Sites: wolfcrypt/src/wc_mldsa.c:262; wolfcrypt/src/wc_mldsa.c:258-268
WC_MLDSA_HAVE_CHECK_KEY is WOLFSSL_MLDSA_CHECK_KEY || (!NO_CHECK_KEY && WC_MLDSA_HAVE_MAKE_PUBLIC_KEY && WC_MLDSA_FAULT_HARDEN && !MAKE_KEY_SMALL_MEM). The second arm requires !WOLFSSL_MLDSA_NO_MAKE_KEY via WC_MLDSA_HAVE_MAKE_PUBLIC_KEY, and wolfssl/wolfcrypt/dilithium.h:317-323 derives WOLFSSL_MLDSA_PUBLIC_KEY and WOLFSSL_MLDSA_PRIVATE_KEY from exactly that, then auto-defines WOLFSSL_MLDSA_CHECK_KEY at dilithium.h:325-329 whenever both are set and WOLFSSL_MLDSA_NO_CHECK_KEY is not. The second arm is therefore strictly subsumed by the first, and WC_MLDSA_HAVE_CHECK_KEY is always identical to WOLFSSL_MLDSA_CHECK_KEY. The same condition is hand-copied into wolfssl/wolfcrypt/wc_mldsa.h:806-810 in a different form (expanding WC_MLDSA_HAVE_MAKE_PUBLIC_KEY inline and dropping !WOLFSSL_MLDSA_VERIFY_ONLY); the two are equivalent today only because VERIFY_ONLY force-defines NO_MAKE_KEY at dilithium.h:310-312 - precisely the fragile coupling the new macro was meant to remove. The PR comment One macro each so the guards can't drift is not achieved here.

Also, at wolfcrypt/src/wc_mldsa.c:258-268 (Info, bugs scan) — WC_MLDSA_HAVE_CHECK_KEY second arm is unreachable; small-mem still compiles CheckKey contrary to its comment:

The new WC_MLDSA_HAVE_CHECK_KEY macro is introduced with the stated goal that "Small-mem re-derives streaming instead, so it never pulls CheckKey (and full matrix A) in." That goal is not achieved, because the second arm of the macro can never be independently true. wolfssl/wolfcrypt/dilithium.h:317-330 already auto-defines WOLFSSL_MLDSA_CHECK_KEY whenever PUBLIC_KEY && PRIVATE_KEY && !WOLFSSL_MLDSA_NO_CHECK_KEY, and !WOLFSSL_MLDSA_NO_MAKE_KEY (required by WC_MLDSA_HAVE_MAKE_PUBLIC_KEY) implies both. So arm B implies arm A, WC_MLDSA_HAVE_CHECK_KEY is identically equal to WOLFSSL_MLDSA_CHECK_KEY, and every guard substitution in this PR is a semantic no-op. A WOLFSSL_MLDSA_MAKE_KEY_SMALL_MEM build still compiles wc_MlDsaKey_CheckKey() along with its full matrix-A allocation (up to ~57 KB for ML-DSA-87), which is the exact footprint the macro was added to avoid. The same condition is hand-copied into wolfssl/wolfcrypt/wc_mldsa.h:806-812 (headers cannot see the .c-only macro), creating two copies that must be kept in sync for no benefit. No runtime misbehavior — the small-mem streaming re-derivation is still what executes — but the code-size promise in the comment is not delivered.

Suggestion:

Recommendation: Drop the dead second arm and revert the wc_mldsa.h prototype guard, so there is one condition rather than three spellings of the same thing.

[wolfcrypt/src/wc_mldsa.c:258-268] Either drop the redundant macro and keep using WOLFSSL_MLDSA_CHECK_KEY directly (removing the duplicated condition from wc_mldsa.h:806-812), or — if the small-mem size saving is actually wanted — make the exclusion real by having dilithium.h stop auto-defining WOLFSSL_MLDSA_CHECK_KEY under WOLFSSL_MLDSA_MAKE_KEY_SMALL_MEM, so that:

#define WC_MLDSA_HAVE_CHECK_KEY

can genuinely be false in a small-mem build. Until one of those happens, correct the comment so it does not claim a code-size reduction that is not delivered.

🔵 [Low] Doxygen for PublicKeyToDer/KeyToDer states the derivation is unconditional, but the code gates it on output != NULL
🔧 NIT style

The added blocks for wc_MlDsaKey_PublicKeyToDer (doc line 909) and wc_MlDsaKey_KeyToDer (doc line 941) say "If only the private key is set, the public key is derived and cached in key" and add MEMORY_E/PUBLIC_KEY_E returns, with no mention that the size query is exempt. The implementation deliberately skips the derive when output == NULL (wc_mldsa.c:14398 and wc_mldsa.c:14513), precisely so a size query stays cheap and non-mutating, and neither error can be returned in that case. The equivalent block for wc_MlDsaKey_ExportPubRaw (doc line 787) is accurate because that function has no size-query mode.

Suggestion: If only the private key is set and output is non-NULL, the public key is
derived and cached in key; don't share key across threads during such a
call. A size query (output == NULL) never derives and never mutates key.

Recommendation: Qualify both blocks with the output != NULL condition so the documented error set matches the size-query path.

🔵 [Low] placeholder byte relies on an undocumented internal contract of SetAsymKeyDer
🔧 NIT style

Both wc_MlDsaKey_PublicKeyToDer (wc_mldsa.c:14463) and wc_MlDsaKey_KeyToDer (wc_mldsa.c:14522) pass a one-byte stack object as pubKey alongside a pubKeyLen of up to 2592, relying on the callee not reading it. I verified this holds: SetAsymKeyDerPublic (wolfcrypt/src/asn.c:13389) only reaches its XMEMCPY(output, pubKey, pubKeyLen) inside if ((ret == 0) && (output != NULL)), and SetAsymKeyDer (wolfcrypt/src/asn.c:34327) only touches pubKey inside if (ret == 0 && output != NULL) in both the template and non-template branches. But neither function's header comment states that contract, so a future change reading pubKey during the sizing pass would become a 2591-byte stack over-read with no compiler or test signal.

Suggestion: Add to the doxygen of SetAsymKeyDer() and SetAsymKeyDerPublic() in wolfcrypt/src/asn.c:

• @note pubKey must be non-NULL but is only dereferenced when output is

•   non-NULL; callers performing a size query may pass a placeholder.
  

Recommendation: Document the "pubKey is not read when output == NULL" contract at both callees so the placeholder trick is protected by more than a comment at the call site.

🔵 [Low] New key->ku scrub in ecc_make_pub_ex() is skipped on async WC_PENDING_E and never re-run
🔧 NIT

This PR adds a scrub of key->ku - the unblinded private scalar that ecc_get_k() reconstructs from k ^ kb under WOLFSSL_ECC_BLIND_K - at the end of ecc_make_pub_ex(). The scrub is explicitly skipped when the operation returns WC_PENDING_E, and nothing re-runs it: the PR's own comment two blocks above states the async result lands directly in key->pubkey and "the operation is not re-issued", and mp_forcezero(key->ku) appears nowhere else in ecc.c except wc_ecc_free() (line 8592). So on the async path the unblinded scalar stays resident in the key struct for the whole key lifetime, which is exactly what WOLFSSL_ECC_BLIND_K exists to prevent. The exposure is slightly wider than the sp_int digits: ecc_make_pub_sw() at line 5560 calls wc_mp_to_bigint_sz(ecc_get_k(key), &ecc_get_k(key)->raw, keySz) before IntelQaEccPointMul(), copying the plain scalar into the ku->raw heap bigint for the hardware; sp_forcezero() clears a->raw via wc_bigint_zero() (sp_int.c:5395), so the skipped scrub also skips that copy. Reachable with WOLFSSL_ASYNC_CRYPT + WC_ASYNC_ENABLE_ECC_KEYGEN + HAVE_INTEL_QA + WOLFSSL_ECC_BLIND_K, and this PR newly routes every default wc_EccPrivateKeyDecode() through ecc_make_pub_ex(), so the path is hit once per key decode rather than only at keygen. Rated Low: exploitation requires local memory-forensics access, and other ECC operations already leave ku populated on their own (pre-existing), so the marginal exposure over the status quo is small - but the newly added mitigation is incomplete on a reachable path.

Recommendation: Scrub key->ku on the async-completion path as well, so the pending case is covered rather than permanently skipped. Either call mp_forcezero(key->ku) from the async resume/completion handler that harvests key->pubkey, or - since ecc_get_k() recomputes ku from k ^ kb on every call and the QAT operation reads ku->raw, not ku->dp - clear ku->dp immediately after wc_mp_to_bigint_sz() and clear ku->raw in the completion path. At minimum, document in the comment that the async path defers the scrub to wc_ecc_free() so the gap is a known, deliberate limitation.

⚪ [Info] Redundant guard terms in tests/suites.c and the new ML-DSA test guards
🔧 NIT style

The new suites.c guard rationale checks out against tests/test-tls13-slhdsa-shake.conf and tests/test-tls13-slhdsa-sha2.conf: both sides do use ML-DSA-44 entity certs, so sign and verify are both needed, and the SLH-DSA root is only ever verified. WOLFSSL_MLDSA_NO_SIGN and WOLFSSL_MLDSA_NO_VERIFY are real macros. However, now that !defined(WOLFSSL_MLDSA_NO_SIGN) is present, the pre-existing !defined(WOLFSSL_MLDSA_VERIFY_ONLY) term is redundant - dilithium.h:310-312 force-defines WOLFSSL_MLDSA_NO_SIGN whenever VERIFY_ONLY is set. The same pattern appears in the new tests: test_mldsa_private_key_export_derives_public_key (tests/api/test_mldsa.c:7934) nests an inner !defined(WOLFSSL_MLDSA_NO_ASN1) && defined(WOLFSSL_MLDSA_PUBLIC_KEY) that merely restates its own outer guard, and several new guards spell out defined(WOLFSSL_MLDSA_PRIVATE_KEY) alongside !defined(WOLFSSL_MLDSA_NO_MAKE_KEY), which implies it via dilithium.h:321-323.

Suggestion: Drop !defined(WOLFSSL_MLDSA_VERIFY_ONLY) && from both suites.c guards, and drop the redundant inner #if at tests/api/test_mldsa.c:7934.

Recommendation: Trim the redundant terms so each guard states its condition once; this is the same drift risk as the WC_MLDSA_HAVE_CHECK_KEY finding.